remove MSI package and replace with app-local VSIX packages #4819

brettfo · 2018-05-02T18:15:33Z

This completely removes our MSI package and replaces it with new V3 VSIX packages which makes us fully side-by-side compliant.

I tested the following scenarios:

Side-by-side install of VS 15.7 and a custom build with these changes.
- Verified that the compiler files dropped by the 15.7 MSI weren't touched during the test build install.
- From the 15.7 developer command prompt:
  - %FSHARPINSTALLDIR% = C:\Program Files (x86)\Microsoft SDKs\F#\10.1\Framework\v4.0\ (set by the current MSI)
  - where fsc reports the above location
  - msbuild test.fsproj shows fsc being invoked from that location for both legacy and SDK style projects
  - msbuild reported pulling FSharp.Core.dll from %ProgramFiles(x86)%\Reference Assemblies\....
  - dotnet build uses its own fsc as expected
- From the test build developer command prompt:
  - %FSHARPINSTALLDIR% = C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\15.0\Bin\FSharp\ (new location for the VSIX)
  - where fsc reports the above location
  - msbuild test.fsproj shows fsc being invoked from that location for both legacy and SDK style projects
  - msbuild reported pulling FSharp.Core.dll from %VSINSTALLDIR%\MSBuild\15.0\Bin\FSharpSdk thanks to the redirection in Microsoft.FSharp.targets.
  - dotnet build uses its own fsc as expected
- From VS 15.7:
  - Building a project shows fsc as being invoked from C:\Program Files (x86)\Microsoft SDKs\F#\10.1\Framework\v4.0\ for both desktop and SDK style projects
- From the test install of VS:
  - Building a project shows fsc as being invoked from C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\15.0\Bin\FSharp\ for both desktop and SDK style projects
Removed VS 15.7 and re-ran all test build scenarios above to ensure nothing was getting pulled from there.
Removed everything and installed the BuildTools version with F# support and verified all command line scenarios:

abelbraaksma · 2018-05-03T14:15:19Z

Perhaps important to note that it should remain part of VS MSBuild tools. I reported that F# 4.0 is missing there, but new releases should continue to be part of that, I hope, since it's the go to method used on build servers

brettfo · 2018-05-08T18:19:11Z

@abelbraaksma I verified the BuildTools scenario with the internal build I generated and it worked as expected. I updated the PR description with all scenarios I tested as well as the checkbox I used for the BuildTools scenario.

Pilchie · 2018-05-08T23:13:42Z

@brettfo For the SxS case, does that rely on nothing else changing the MSI? If we have to update something else, won't that mean that the previous builds will stop working when the new MSI is installed?

Pilchie · 2018-05-08T23:15:06Z

build/config/InsertionSignToolData.json

Why are the above excluded - that doesn't seem right.

The listed files have either already been signed during this build (fsi, etc.) or come from a nuget package (microsoft.build.*).

Can it be commented somehow so that people know why these are excluded?

Sadly, no. The JSON file format specification doesn't allow comments as per RFC 7159.

Pilchie · 2018-05-08T23:16:38Z

setup/Swix/Microsoft.FSharp.Compiler/Files.swr

We had some problems about ngen priority with Roslyn - is this right?

I'll talk to some folks on the compiler team to see what issues they had.

KevinRansom

Gosh this looks good.

abelbraaksma · 2018-05-09T19:45:23Z

@brettfo , that sounds great! If there's something specifically you'd want me to test, let me know. I'm traveling, but I'll try to get to it sooner rather than later.

brettfo · 2018-05-10T23:11:17Z

I spoke with some internal teams about my initial plan of staging this in two PRs, but the number of potential pitfalls is apparently enormous. I've opted instead to do all of the MSI removal in one PR to keep this atomic.

Pilchie

LGTM but I'd like someone from MSBuild to sign off, and I'd like to make sure we check with the setup team about both SxS installs and upgrades. Ideally a straight upgrade would remove the old MSI, but SxS installs wouldn't. Not sure if that's possible though.

Pilchie · 2018-05-11T00:10:02Z

src/fsharp/FSharp.Build/Microsoft.FSharp.Targets

+    path is unchanged from the original legacy templates and if a new reference assembly can be found at the new location.
+
+    -->
+    <Target Name="RedirectFSharpCoreReferenceToNewRedistributableLocation" BeforeTargets="ResolveAssemblyReferences">


I'm not sure how I feel about this target. It's very clever, but may also be very surprising to people, and I feel like various bits of tooling that only use the evaluated MSBuild model will be wrong because they will still use the old hint path.

With that said, I'm not sure of anything better :-/

This was the best we could come up with for not breaking legacy projects, but still removing the MSI from the install toolchain (which unfortunately means we can't have a global, well-known location.)

Hmm. What did the old templates put in user proj files?

Does this do the right thing in design-time builds?

I didn't see any issues with design time builds, but I'll do another pass checking for this specifically.

The old templates created the project files like this:

<ItemGroup> ... <Reference Include="FSharp.Core"> <Name>FSharp.Core</Name> <AssemblyName>FSharp.Core.dll</AssemblyName> <HintPath>$(MSBuildProgramFiles32)\Reference Assemblies\Microsoft\FSharp\.NETFramework\v4.0\$(TargetFSharpCoreVersion)\FSharp.Core.dll</HintPath> </Reference> ... </ItemGroup>

and we want to make sure we don't break those scenarios.

Pilchie · 2018-05-11T00:11:25Z

setup/Swix/Microsoft.FSharp.SDK/Files.swr

        version=$(FSharpPackageVersion)

-folder "InstallDir:Common7\IDE\CommonExtensions\Microsoft\FSharpSdk"
+folder "InstallDir:MSBuild\15.0\Bin\FSharpSdk"


@rainersigwald - is this the right sort of convention here?

I copied the Roslyn behavior for this.

I would not expect F# to need to be in the MSBuild bin folder. C# is special because we both used to be in the framework. Plus, people often construct relative paths, so changing it can break folks. Did something break with the old path?

The original location from the MSI was C:\Program Files (x86)\Microsoft SDKs\FSharp\.... My first round of this PR moved the compiler to Common7\IDE\CommonExtensions\Microsoft\FSharpSdk, but that felt wrong for the BuildTools SKU because there is no IDE. The second round of this PR I moved it to be under MSBuild to copy C#. Do you have a suggestion for a better place to put it?

I would agree with @rainersigwald. I don't this this is an acceptable location, especially given that you're redistributing our old binaries. Common7\IDE\CommonExtensions\Microsoft\FSharpSdk seems fine with me, that's where NuGet is and they're required in no-IDE scenarios.

Pilchie · 2018-05-11T00:12:20Z

setup/resources/Microsoft.FSharp.NetSdk.Shim.props

 <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

-  <Import Project="$(MSBuildThisFileDirectory)..\..\..\..\..\Common7\IDE\CommonExtensions\Microsoft\FSharpCompiler\Microsoft.FSharp.NetSdk.props" />
+  <Import Project="$(VsInstallRoot)\MSBuild\15.0\Bin\FSharp\Microsoft.FSharp.NetSdk.props" />


Can VsInstallRoot be counted on to exist?

According to this that value will always be present, but reading the comment it could behave oddly with $(PlatformToolset). Would you prefer I go back to the $(MSBuildThisFileDirectory)..\..\..\etc method? It doesn't look as clean, but will always produce consistent results.

PlatformToolset is a pretty C++ concept, so I'm not too worried about that. However, I do wonder about things like the build sku. I'll defer to @rainersigwald or @AndyGerlicher for what should happen here.

I also tested on a clean machine with only the BuildTools version installed. Running msbuild MyProject.fsproj showed the correct compiler and FSharp.Core being used which means these targets redirected appropriately.

brettfo · 2018-05-11T00:22:45Z

@Pilchie Regarding your comment about SxS installs, I verified that I could install/uninstall 15.7 and my custom build in any order. 15.7 always removed the MSI when it was uninstalled, but that never interfered with my custom build. From the internal email conversation I gathered that upgrade would trigger an uninstall.

Pilchie · 2018-05-11T00:25:30Z

I inferred that upgrade would trigger an uninstall/reinstall if the new version contains the MSI, but what happens if the new version doesn't?

Would a straight uninstall be a problem if someone upgrades Preview channel from P2 (with MSI) to P3 (without MSI), while also having 15.7 installed, or would ref-counting deal with that.

I think things should work out, but I think we have some more testing to consider - or at least thought experiments involving people who know more about setup than me :)

brettfo · 2018-05-11T16:50:04Z

It appears our existing MSI is properly ref-counted so the scenario of having multiple instances installed and upgrading one of them (which will in turn /uninstall the MSI) should work. I simulated this by checking the following:

Verify no F# SDK is installed via Get-WmiObject -Class win32_product | Where-Object ($_.Name -Match ".*F#.*"}.
Install 15.7 Community => 1 copy of the SDK is installed as per step 0.
Install 15.7 Enterprise => still 1 copy of the SDK, e.g., a duplicate copy wasn't installed.
Uninstall 15.7 Community => still 1 copy of the SDK, e.g., it wasn't removed even though the corresponding install is what initially put it there.
Uninstall 15.7 Enterprise => 0 copies of the SDK.

brettfo · 2018-05-14T16:16:14Z

@Pilchie, did you have any other questions regarding this PR? I think I covered your issues above, although maybe not with enough depth.

Pilchie · 2018-05-14T16:25:32Z

LGTM, but I would feel more comfortable if we can get someone from the setup team to take a look, and would really like @rainersigwald, or another MSBuild dev to take a look too.

rainersigwald

Moving into the MSBuild folder is my biggest concern.

rainersigwald · 2018-05-14T18:37:05Z

setup/Swix/Microsoft.FSharp.Compiler/Files.swr

+  file source="$(BinariesFolder)\net40\bin\fsiAnyCpu.exe.config"
+  file source="$(BinariesFolder)\net40\bin\Microsoft.Build.Conversion.Core.dll"
+  file source="$(BinariesFolder)\net40\bin\Microsoft.Build.dll"
+  file source="$(BinariesFolder)\net40\bin\Microsoft.Build.Engine.dll"


Are you redistributing MSBuild assemblies?

Yes. Our current MSI ships these, too. If you have F# installed on your machine these assemblies live next to our compiler at C:\Program Files (x86)\Microsoft SDKs\F#\10.1\Framework\v4.0.

@rainersigwald. Unfortunately yes. The F# compiler needs this, it's something we are going to try to eliminate eventually, but it's not going to happen quickly.

rainersigwald · 2018-05-14T18:41:16Z

src/fsharp/FSharp.Build/Microsoft.FSharp.Targets

+
+      <ItemGroup>
+        <!-- Update references to `.NETCore\*\FSharp.Core.dll`. -->
+        <Reference Update="%(Reference.Identity)"


Don't use Update= inside a target . . . it doesn't do the right thing. dotnet/msbuild#2835

Can you elaborate? I read the attached issue, but wasn't able to follow. In my testing the Update= seemed to do the right thing; the <HintPath> for FSharp.Core was appropriately redirected.

rainersigwald · 2018-05-14T18:42:39Z

src/fsharp/FSharp.Build/Microsoft.FSharp.Targets

+    path is unchanged from the original legacy templates and if a new reference assembly can be found at the new location.
+
+    -->
+    <Target Name="RedirectFSharpCoreReferenceToNewRedistributableLocation" BeforeTargets="ResolveAssemblyReferences">


Hmm. What did the old templates put in user proj files?

Does this do the right thing in design-time builds?

rainersigwald · 2018-05-14T18:43:41Z

src/fsharp/FSharp.Build/Microsoft.FSharp.Targets

+      <ItemGroup>
+        <!-- Update references to `.NETCore\*\FSharp.Core.dll`. -->
+        <Reference Update="%(Reference.Identity)"
+                   Condition="'%(Reference.Identity)' == 'FSharp.Core' and


Does it matter if this misses references that were referenced by full assembly identity?

These updates are only to catch project files that never changed the default values, and the F# templates (to my knowledge) have never used the full assembly identity. If a developer modifies this value then they're on their own for locating FSharp.Core.

AndyGerlicher · 2018-05-16T02:00:39Z

I really wish this could be moved to a NuGet package or MSBuild SDK that could just be acquired a build-time (or local offline feed) :(

I realize that would likely change project files to work though so it might be out. At minimum though, this shouldn't go under MSBuild bin folder. $(MSBuildExtensionsPath) is the legacy style location for finding stuff that VS magically lays out on your machine, so if you can't go with something outside of MSBuild's folder I would key off of that. Maybe $(MSBuildExtensionsPath)\Microsoft\FSharp\?

brettfo · 2018-05-16T17:41:08Z

I've changed the location of the F# compiler back to InstallDir:Common7\IDE\CommonExtensions\Microsoft\FSharpCompiler and updated the shim targets to use this exact path instead of a relative path.

brettfo · 2018-05-17T21:57:34Z

@AndyGerlicher @rainersigwald I've moved the compiler to InstallDir:Common7\IDE\CommonExtensions\Microsoft\FSharpCompiler. Did either of you have any other concerns?

brettfo · 2018-05-23T17:57:45Z

Ping to @rainersigwald. You mentioned above that Update= shouldn't be used in a target, can you tell me why? I wasn't able to discern the problem via the link you originally provided. In my local testing the Update= calls executed in the target without issue and the references were appropriately redirected. I verified this by setting the build output to Normal and Ctrl+Shift+B and verifying the reference passed to fsc.exe in the output window.

rainersigwald · 2018-05-23T17:59:42Z

Ah, sorry. Update shouldn't be used in an item in a target because it is ignored. It can work in some cases if you're not actually attempting to update a subset of uses, but you shouldn't rely on that; we'd like to make it an error in the future.

brettfo · 2018-05-23T23:21:44Z

Update doesn't work in a target, but can I use Include and Remove? Or are those a similar mechanism/also ignored?

rainersigwald · 2018-05-24T00:54:54Z

Include and Remove are fine. When we added Update to outside-a-target items, we missed a case inside targets that should have been accounted for.

brettfo · 2018-05-24T20:40:10Z

@rainersigwald I re-worked Microsoft.FSharp.targets to use only Remove and Include instead of Update, can you take another peek at it? If it makes the diff easier it's the most recent commit on this PR.

Edit, I spoke with Rainer offline and it turns out I just need to remove the Update= lines from the target.

brettfo · 2018-05-25T16:46:46Z

Final validation build (with removed Update= attributes) shows that references are getting rewritten as expected. Last call for any final comments, I'll leave this open for a few hours.

brettfo force-pushed the compiler-vsix branch 2 times, most recently from 4a070eb to a6ef545 Compare May 2, 2018 23:06

brettfo force-pushed the compiler-vsix branch 7 times, most recently from 1edfc0b to 17e131f Compare May 8, 2018 18:07

brettfo changed the title ~~[WIP] move compiler out of MSI and into a VSIX~~ move compiler out of MSI and into a VSIX May 8, 2018

brettfo force-pushed the compiler-vsix branch 2 times, most recently from 09d3550 to fff1707 Compare May 8, 2018 22:52

Pilchie approved these changes May 8, 2018

View reviewed changes

KevinRansom approved these changes May 8, 2018

View reviewed changes

brettfo force-pushed the compiler-vsix branch from fff1707 to 2482483 Compare May 10, 2018 23:05

brettfo changed the title ~~move compiler out of MSI and into a VSIX~~ remove MSI package and replace with app-local VSIX packages May 10, 2018

Pilchie approved these changes May 11, 2018

View reviewed changes

rainersigwald suggested changes May 14, 2018

View reviewed changes

brettfo force-pushed the compiler-vsix branch from 2482483 to 91ca3b8 Compare May 16, 2018 17:34

brettfo force-pushed the compiler-vsix branch 2 times, most recently from 992b6c1 to f77094b Compare May 16, 2018 17:40

brettfo mentioned this pull request May 17, 2018

Add the compiler to the regular IDE package. #4922

Merged

KevinRansom mentioned this pull request May 17, 2018

allow FSharpTargetsPath to be properly overwritten in a project file #4934

Merged

cartermp added this to the 15.8 milestone May 20, 2018

brettfo added 4 commits May 24, 2018 13:18

move compiler out of MSI and into a VSIX

10abc13

move redistributable SDK out of MSI and into a VSIX

80e860c

rename SWIX packages and remove unnecessary SDK resources package

ceffd87

use a more deterministic path for targets shims

f8897dd

brettfo force-pushed the compiler-vsix branch from f77094b to e58917c Compare May 24, 2018 20:37

brettfo force-pushed the compiler-vsix branch from e58917c to 1fa5040 Compare May 24, 2018 20:44

remove unnecessary Update verb

2e9e6da

brettfo force-pushed the compiler-vsix branch from 1fa5040 to 2e9e6da Compare May 24, 2018 20:58

rainersigwald approved these changes May 24, 2018

View reviewed changes

brettfo merged commit 958bfea into master May 25, 2018

brettfo deleted the compiler-vsix branch May 25, 2018 21:30

brettfo mentioned this pull request May 30, 2018

split swix packages into *.Compiler and *.IDE #5034

Merged

remove MSI package and replace with app-local VSIX packages #4819

remove MSI package and replace with app-local VSIX packages #4819

Uh oh!

Conversation

brettfo commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abelbraaksma commented May 3, 2018

Uh oh!

brettfo commented May 8, 2018

Uh oh!

Pilchie commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinRansom left a comment

Choose a reason for hiding this comment

Uh oh!

abelbraaksma commented May 9, 2018

Uh oh!

brettfo commented May 10, 2018

Uh oh!

Pilchie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettfo commented May 11, 2018

Uh oh!

Pilchie commented May 11, 2018

Uh oh!

brettfo commented May 11, 2018

Uh oh!

brettfo commented May 14, 2018

Uh oh!

Pilchie commented May 14, 2018

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettfo commented May 2, 2018 •

edited

Loading

brettfo commented May 24, 2018 •

edited

Loading